Enabling the use of gurobipy directly, which should be faster #60
Conversation
|
|
||
| # choose default from installed choices | ||
| if 'glpk' in installed_solvers: | ||
| if 'gurobi' in installed_solvers: | ||
| default_solver = 'gurobi' |
There was a problem hiding this comment.
I lean towards not making gurobi a default solver, because gurobi and gurobipy are proprietary, closed-source software: https://www.gurobi.com/downloads/end-user-license-agreement-academic/, and: https://pypi.org/project/gurobipy/.
There was a problem hiding this comment.
I agree with not making gurobi a default solver. This is not a vote against Gurobi, but rather, automatically using a restrictive-license-encumbered dependency is an anti-pattern of open source code. Users should have to explicitly request it.
Perhaps there is a more convenient way for users to do this, e.g., an environment variable.
| @@ -1 +1 @@ | |||
| cvxopt==1.2.4 | |||
| cvxopt==1.2.4 | |||
There was a problem hiding this comment.
This line appears to have only whitespace changes. It seems that the line could be omitted from the changeset.
| @@ -88,6 +95,8 @@ def lpsolve(c, G, h, solver=None): | |||
| result = _solve_lp_using_cvxopt(c, G, h, solver=solver) | |||
| elif solver == 'scipy': | |||
| result = _solve_lp_using_scipy(c, G, h) | |||
| elif solver == 'gurobi': | |||
There was a problem hiding this comment.
Passing the selection 'gurobi' as a keyword argument as done here is explicit [PEP 8] and seems preferable.
| @@ -486,5 +486,14 @@ def is_glpk_present(): | |||
| return False | |||
|
|
|||
|
|
|||
| def test_gurobipy_return_same_result_as_scipy(): | |||
There was a problem hiding this comment.
It is good to test the added functionality. However, the absence of the solver gurobi on Travis CI implies that this test fails. Skimming the licensing webpage of gurobi gives the impression that installing gurobi on Travis CI is probably far from its licensing procedure. So it seems that this test would need to not be invoked on Travis CI.
|
@RichardOberdieck Thanks for this pull request. I will try to install Gurobi and provide requirements before this can be accepted. My review is arriving much later than I would have wanted, so no worries if you do not have time to handle comments. It seems that GitHub allows me to push to your pull request branch, so I can do remaining work, too. |
|
Thanks @slivingston : I am a bit swamped at the moment, but it looks better towards August and September. If some things are outstanding until then, please let me know! Thanks for picking this up again! |
|
@RichardOberdieck thanks for this code. Because additional work was required to prepare this for merging, I opened a new pull request that applies changes onto your original commit. |
|
@RichardOberdieck Also, I opened a new PR because it did not appear that I can push changes to this PR (despite seeming to be able to previously). Your authorship will be captured when merging the new PR either as the commit author or via |
|
@slivingston thanks for your work on this, truly diligent! Thanks for any attribution, although after reviewing my code changes I am not sure they warrant a "co-authored". But I leave that decision up to you! |
For discussion that concluded with this commit, #98 #60 Gurobi will not automatically become the default solver due to its restrictive license. However, users can manually select it by import polytope as pc pc.solvers.default_solver = 'gurobi' Then, `polytope` can be used without modification. If Gurobi returns INF_OR_UNBD, the optimization is run again with DualReductions=0 as recommended at https://www.gurobi.com/documentation/current/refman/optimization_status_codes.html and a notification about this is included under the `"message"` key in the result returned by `polytope.solvers.lpsolve`. Tests that require Gurobi are skipped and a warning is shown if gurobipy is not installed. Co-authored-by: Richard Oberdieck <[email protected]>
I've added support for gurobipy in the solvers, and added a test that works fine. Let me know if you need anything else.